Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added target_os cfg feature to header translator #433

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

simlay
Copy link
Collaborator

@simlay simlay commented Apr 2, 2023

I was looking at #408 and this seemed like a nice small unit of work to do on this. I'm not sure what to do with the ios_app_extension and macos_app_extension booleans in the Unavailable struct. To disallow the maccatalyst targets, the nice way to do it is with the not yet stabilized target_abi feature.

@simlay
Copy link
Collaborator Author

simlay commented Apr 2, 2023

Hmm. Looks like the submodule diff in CI is failing as this generates a new things.

@madsmtm
Copy link
Owner

madsmtm commented Apr 3, 2023

Hmm. Looks like the submodule diff in CI is failing as this generates a new things.

Yeah, that's intentional - you should be able to fork the icrate-generated repo and push a commit with the changes there, but if that's troublesome (e.g. if you don't have the correct SDK version) I can do it for you?

@simlay
Copy link
Collaborator Author

simlay commented Apr 3, 2023

Well, I updated the submodule ref to a my branch and it's obviously not there. I guess the better way to do this is to merge madsmtm/objc2-generated#6 and then update the ref.

@madsmtm
Copy link
Owner

madsmtm commented Apr 4, 2023

The result looks nice so far, two things though:

  1. Could we perhaps use the data from Config.libraries to not emit the attributes when not needed (e.g. AppKit is littered with redundant #[cfg(not(any(target_os = "ios")))]).
  2. The reason the CI is failing is because the attribute isn't added in generated/[Framework]/mod.rs yet.

Well, I updated the submodule ref to a my branch and it's obviously not there

I think it worked? Also, you don't need to submit PRs to icrate-generated, I can find them without ;) (and I won't be doing a merge anyhow, just a fast-forward, to keep the size of the repo as minimal as possible while preserving the refs).

maccatalyst

We'll probably need a build script for that in the meantime like we do for objc-sys. I'm fine with it not happening in this PR though.


Sorry for the hazzle btw, I haven't gotten around to writing better docs for how to contribute to header-translator, so it's a bit confusing atm.

@simlay
Copy link
Collaborator Author

simlay commented Apr 4, 2023

maccatalyst

We'll probably need a build script for that in the meantime like we do for objc-sys. I'm fine with it not happening in this PR though.

Ah yes. rust-lang/rust#96901 is the not yet stabilized feature for future reference.

Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked it through, it's really good so far, and the output is definitely an improvement!

A few of these are nits, but since they don't interfere with the actual functionality, including the doc_auto_cfg feature (try it with +nightly --features=unstable-docsrs, they're not that important.

Also, I think you have a newer Xcode version than the one icrate is currently built with (which is Xcode 14.2 as pr. the README), which is why the build is failing. I'll try to find some time to update it this week, or perhaps you can do it? (in a separate PR, for the CI part change the DEVELOPER_DIR env variable)


I realize I may be asking a lot here, if you don't feel like doing some of the work I've outlined, it's perfectly fine, your contribution is still very valuable as-is!

crates/header-translator/src/stmt.rs Outdated Show resolved Hide resolved
crates/header-translator/src/availability.rs Show resolved Hide resolved
.collect::<Vec<String>>()
.join(",");
if !unavailable_oses.is_empty() {
write!(f, "#[cfg(not(any({unavailable_oses})))]")?;
Copy link
Owner

@madsmtm madsmtm Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that the headers expose this as "unavailability", but I think it would look nicer in the docs if it was inverted; e.g. that #[cfg(not(any(target_os = "watchos")))] became #[cfg(any(target_os = "tvos", target_os = "ios", target_os = "macos"))] instead (this is also how Apple's own docs does it).

This interferes with GNUStep of course, but perhaps we can just code in a manual override there for all of AppKit and Foundation for now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually spent a little bit of time doing both variants - all(not(target_os = "ios"), any(taget_os = "macos", target_os = "tvos", target_os = "watchos")). I didn't like it as then everything had a cfg attribute.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with that part

@@ -29,20 +102,27 @@ struct Versions {

#[derive(Debug, Clone, PartialEq)]
pub struct Availability {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[deprecated] now also appears on impl ClassType, which is incorrect.

So perhaps we should split out the Unavailable from Availability (and maybe rename it to PlatformCfg)? Since that part is always going to be a cfg-gate, which has interactions with higher-level cfg-gates (and must propagate to imports like generated/[Framework]/mod.rs), while introduced and deprecated will always only need to apply to the statement/method/field they're associated with.

Maybe it would even make sense to add PlatformCfg as a field of ItemIdentifier? Since these two are access so often together.

crates/header-translator/src/library.rs Show resolved Hide resolved
@@ -269,7 +269,7 @@ fn parse_sdk(index: &Index<'_>, sdk: &SdkPath, llvm_target: &str, config: &Confi
preprocessing = false;
// No more includes / macro expansions after this line
let file = library.files.get_mut(&file_name).expect("file");
for stmt in Stmt::parse(&entity, &context) {
for stmt in Stmt::parse(&entity, &context, &library.unavailability) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing the (un-)availability around everywhere like this, I think it would be fine to introduce a few optional fields in Context<'_> that carried this information (since the context is already passed everywhere).

This would also be useful for other things in the future.

So something like (you can freely add only the stuff you need now, just expanding on the example to show the idea):

struct Context<'a> {
    // ... Existing fields

    /// Set whenever we know what the current library is.
    current_library: Cell<Option<Arc<Library>>>,
    /// Set when parsing methods, struct fields, enum fields, ...
    current_type: Cell<Option<(ItemIdentifier, Unavailability)>>,
}

impl Context<'_> {
    fn library(&self) -> Arc<Library> {
        self.current_library.clone().take()
    }

    fn in_library<R>(&self, library: Arc<Library>, f: impl FnOnce(&Self) -> R) -> R {
        self.current_library.set(Some(library));
        let result = f(self);
        self.current_library.set(None);
        result
    }

    // ... Similar methods for the others
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did part of this by adding current_unavailability to Context. In this specific case library is a &mut Library and so passing it in as even a Arc<Library> would require a clone (I think?). Library doesn't derive(Clone)and I didn't feel like adding that. It's definitely cleaner but I'm not in love with it.

Comment on lines +1412 to +1413
let unavailable = availability.unavailable.clone();
write!(f, "{unavailable}")?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go inside the extern_static! call, and the macro itself in icrate/src/macros.rs should instead be updated to allow it - otherwise, the cfg attribute will be improperly picked up by auto_doc_cfg (it currently works though because generated/[Framework]/mod.rs re-applies the attribute).

Same goes for typed_enum! and typed_extensible_enum!.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, reflecting on this a bit more, perhaps we should instead aim to move as many cfg attributes as we can to outside macro invocations, to allow the compiler to skip evaluating the macro at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, reflecting on this a bit more, perhaps we should instead aim to move as many cfg attributes as we can to outside macro invocations, to allow the compiler to skip evaluating the macro at all?

Yeah, I went back and forth on where to put the cfg attributes and felt that outside the macro was better. I should make sure it's consistent though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some unclear reason, after updating macOS, I ended up with errors on the enum macros so I put the cfg attributes on all the macros (I think). I had to use just the unavailable part as the #[deprecated] stuff before the macro causes clippy lint issues. I might try and re-add them.

# The MPMediaQueryAddition definition does not have API_UNAVAILABLE annotations
# where as the MPMediaItem does. This results in problematic conditional compliation:
# https://github.com/phracker/MacOSX-SDKs/blob/041600eda65c6a668f66cb7d56b7d1da3e8bcc93/MacOSX11.3.sdk/System/Library/Frameworks/MediaPlayer.framework/Versions/A/Headers/MPMediaQuery.h#L103-L118
[class.MPMediaItem.categories.MPMediaQueryAdditions]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this is not the only case (it's just the only one caught since we're compiling for macOS), perhaps it would make sense to change Stmt::Methods::category to Option<(ItemIdentifier<Option<String>>, Availability)>?

This is also an argument for adding Unavailable/PlatformCfg to ItemIdentifier, then the problem would neatly go away.

@@ -579,7 +579,7 @@ impl Stmt {
cls: id.clone(),
generics: generics.clone(),
category: ItemIdentifier::with_name(None, entity, context),
availability: Availability::parse(entity, context),
availability: availability.clone(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that this is the correct way to deal with this. I noticed the generated cfg attributes were gone for superclass functions. I'm guessing that the Availability::parse(entity, context) here checked the availability of the super not the subclass.

@marysaka
Copy link
Contributor

Any news about that PR? What is missing for this to be ready?

I'm quite interested to get this working for some projects.

@simlay
Copy link
Collaborator Author

simlay commented Jul 16, 2023

Any news about that PR? What is missing for this to be ready?

Hmm. Looks like I need to deal with some merge latest conflicts. I think the thing that was most lacking was generated bindings used xcode 14.2 (latest stable) but CI uses an older version.

@madsmtm What do you think?

I'm quite interested to get this working for some projects.

Glad to hear it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants